Skip to content

refactor: split ak._do into meta and content #2852

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

refactor: split ak._do into meta and content #2852

wants to merge 12 commits into from

Conversation

@agoose77
Copy link
Collaborator

@agoose77 agoose77 commented Nov 29, 2023 •
edited
Loading

This PR:

  1. Splits Content._mergeable_next and ak._do.mergeable into Meta._mergeable_next and ak._do.meta.mergeable.
  2. Adds dtype to NumpyMeta
  3. Drops use of Generic in type hints for Meta classes

(3) is required because of the lack of higher-kinded types. This means that our type hints will only let us prove that NumpyArray is a NumpyMeta in some contexts, e.g. via ak._meta.meta.is_numpy()

@codecov
Copy link

codecov bot commented Nov 29, 2023 •
edited
Loading

Codecov Report

Attention: Patch coverage is 92.19858% with 44 lines in your changes are missing coverage. Please review.

Project coverage is 81.95%. Comparing base (123fa09) to head (7d62858).
Report is 107 commits behind head on main.

❗ Current head 7d62858 differs from pull request most recent head dc9645f. Consider uploading reports for the commit dc9645f to get more accurate results

Additional details and impacted files
Files Coverage Δ
src/awkward/__init__.py 97.14% <100.00%> (+0.08%) ⬆️
src/awkward/_connect/jax/trees.py 92.30% <100.00%> (+0.09%) ⬆️
src/awkward/_connect/numpy.py 92.05% <100.00%> (+0.03%) ⬆️
src/awkward/_meta/emptymeta.py 94.11% <100.00%> (+0.36%) ⬆️
src/awkward/_meta/indexedoptionmeta.py 100.00% <100.00%> (ø)
src/awkward/_meta/unionmeta.py 94.11% <100.00%> (+0.07%) ⬆️
src/awkward/_nplikes/array_module.py 86.28% <ø> (-0.10%) ⬇️
src/awkward/_nplikes/numpy_like.py 75.49% <100.00%> (+0.19%) ⬆️
src/awkward/_nplikes/typetracer.py 74.61% <ø> (-0.06%) ⬇️
src/awkward/_pickle.py 85.71% <100.00%> (-1.59%) ⬇️
... and 141 more

@agoose77 agoose77 force-pushed the agoose77/refactor-mergeable-meta branch from 1313cbe to c14a473 Compare November 29, 2023 21:38
@agoose77 agoose77 requested a review from jpivarski November 30, 2023 10:32
jpivarski
jpivarski approved these changes Dec 1, 2023
View reviewed changes
Copy link
Member

@jpivarski jpivarski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that this splits the ak._do module into two submodules for Content and the superclasses of both Content classes and Form classes.

Meta replaces the TypeVar T in many places; I suppose that's because T used to represent a Content or a Form, which had no common superclass, and now it can all be OOP-style, rather than template-style.

The merge* functions have moved. That's fine.

On the whole, there's a lot of differences, but it seems to be just (1) moving things and (2) merging the Content and Form hierarchies. Does this PR complete the merging of those hierarchies, or is there another after this?

Also, what will the hierarchy look like after merging is complete? Is it like this?

flowchart LR
    C(Content)
    F(Form)
    M(Meta)
    ListArray --> C
    NumpyArray --> C
    ListForm --> F
    NumpyForm --> F
    ListMeta --> M
    NumpyMeta --> M
    ListArray --> ListMeta
    NumpyArray --> NumpyMeta
    ListForm --> ListMeta
    NumpyForm --> NumpyMeta
Loading

@agoose77
Copy link
Collaborator Author

agoose77 commented Dec 1, 2023

Meta replaces the TypeVar T in many places; I suppose that's because T used to represent a Content or a Form, which had no common superclass, and now it can all be OOP-style, rather than template-style.

The T was used when I was trying to preserve the idea that ListOffsetArray.content is a Content, not just a Meta. But this ends up needing higher kinded types, which mypy does not yet express.

@agoose77
Copy link
Collaborator Author

agoose77 commented Dec 1, 2023

On the whole, there's a lot of differences, but it seems to be just (1) moving things and (2) merging the Content and Form hierarchies. Does this PR complete the merging of those hierarchies, or is there another after this?

Yes, I think so. We may be able to promote some Content functions to Meta, but on balance I think its complete.

@agoose77 agoose77 force-pushed the agoose77/refactor-mergeable-meta branch from 47290e1 to 7d62858 Compare December 4, 2023 10:57
@jpivarski jpivarski added the pr-inactive A pull request that hasn't been touched in a long time label Mar 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jpivarski jpivarski jpivarski approved these changes

Assignees

No one assigned

Labels

pr-inactive A pull request that hasn't been touched in a long time

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@agoose77 @jpivarski